Skip to content

feat: implement pvc primitive#34

Merged
sourcehawk merged 25 commits intomainfrom
feature/pvc-primitive
Mar 25, 2026
Merged

feat: implement pvc primitive#34
sourcehawk merged 25 commits intomainfrom
feature/pvc-primitive

Conversation

@sourcehawk
Copy link
Owner

@sourcehawk sourcehawk commented Mar 22, 2026

Implements the pvc Kubernetes resource primitive following the pattern established by the existing ConfigMap and Deployment primitives.

Summary

  • Adds pvc primitive package under pkg/primitives/pvc/
  • Implements required lifecycle interfaces
  • Includes editors, mutator, and builder
  • Adds shared PVCSpecEditor under pkg/mutation/editors/
  • Updates docs/primitives.md to include PVC in the built-in primitives table

Checklist

  • Compiles cleanly
  • Tests pass
  • Follows naming conventions in CONTEXT.md

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements a new pvc primitive (PersistentVolumeClaim) that plugs into the framework’s integration-resource lifecycle, including typed mutation/editing support, default handlers, flavors, docs, and a runnable example.

Changes:

  • Added pkg/primitives/pvc/ (resource, builder, mutator, handlers, flavors) with unit tests.
  • Added a shared typed editor PVCSpecEditor under pkg/mutation/editors/ with tests.
  • Added a dedicated pvc-primitive example and new documentation page docs/primitives/pvc.md.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pkg/primitives/pvc/resource.go Adds PVC integration Resource wrapper + default field applicator (immutable-field preservation).
pkg/primitives/pvc/mutator.go Adds PVC mutator with plan/apply pattern and typed spec/meta edits.
pkg/primitives/pvc/mutator_test.go Unit tests for mutator behavior and ordering.
pkg/primitives/pvc/handlers.go Default operational + suspension handlers for PVCs.
pkg/primitives/pvc/handlers_test.go Unit tests for default handlers.
pkg/primitives/pvc/flavors.go PVC flavors for preserving labels/annotations.
pkg/primitives/pvc/flavors_test.go Unit tests for flavors + builder integration check.
pkg/primitives/pvc/builder.go Fluent builder for PVC primitive configuration.
pkg/primitives/pvc/builder_test.go Unit tests for builder configuration/validation wiring.
pkg/mutation/editors/pvcspec.go Adds shared PVCSpecEditor for typed PVC spec mutations.
pkg/mutation/editors/pvcspec_test.go Unit tests for PVCSpecEditor.
examples/pvc-primitive/resources/pvc.go Example resource factory using pvc.Builder + mutations/flavors/extractor.
examples/pvc-primitive/README.md Documentation for running/understanding the PVC example.
examples/pvc-primitive/main.go Runnable fake-client example demonstrating versioning/suspension flows.
examples/pvc-primitive/features/mutations.go Example feature-gated PVC mutations.
examples/pvc-primitive/app/owner.go Re-export of shared ExampleApp types for the example.
examples/pvc-primitive/app/controller.go Example controller wiring the PVC resource into a Component.
docs/primitives/pvc.md Adds primitive documentation for PVC.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:
None

Intentionally ignored:

  • pvcspec.go line 6 (PR description/checklist mismatch): This comment is about PR metadata, not code. The PVCSpecEditor is a new file added to pkg/mutation/editors/ — it extends the shared editors package but does not modify any existing shared files. Adding a new editor alongside the primitive it supports is the intended pattern (consistent with how other primitives work). The PR description could be clarified to note this addition, but no code change is warranted and splitting into a separate PR would lose the cohesion of the feature.

<!-- claude-review-cycle -->

@sourcehawk sourcehawk force-pushed the feature/pvc-primitive branch from 1789aed to 2d07ac1 Compare March 23, 2026 00:49
Copilot AI review requested due to automatic review settings March 23, 2026 00:49
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.

@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • Added PVC primitive entry to the Built-in Primitives table in docs/primitives.md so users can discover the PVC primitive documentation from the main primitives page.
  • Merged latest changes from main into the feature branch.

Intentionally ignored:

  • Double pipe separator comment (pvc.md line 15): The capabilities table already uses single | separators, consistent with configmap.md. No double pipes exist in the file.
  • Mutator beginFeature export comment (mutator.go line 45): The unexported beginFeature() method is the same pattern used by all other primitives (deployment, configmap). The FeatureMutator interface in internal/generic uses an unexported method by design. Changing this would require modifying shared infrastructure and affects all primitives, not just PVC. This is a pre-existing architectural pattern, not a PVC-specific issue.

Copilot AI review requested due to automatic review settings March 23, 2026 16:05
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

Copilot AI review requested due to automatic review settings March 23, 2026 16:58
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • resource.go:32 — PreserveStatus was already added in a prior commit; confirmed present and working
  • resource_test.go:136 — TestDefaultFieldApplicator_PreservesStatus regression test was already added in a prior commit; confirmed present and passing
  • docs/primitives/pvc.md:134 — Updated mutation ordering docs to accurately state that PVC mutations are applied in a single deterministic sequence without per-feature boundaries

Intentionally ignored:

  • mutator.go:51 (FeatureMutator unexported method) — Valid observation, but this is a pre-existing framework-level issue affecting all primitives (deployment, configmap, pvc), not specific to this PR. The unexported beginFeature() in generic.FeatureMutator prevents any external package from satisfying the interface. Should be addressed in a separate framework PR.
  • docs/primitives.md:134 (PR checklist accuracy) — This is about PR description metadata, not a code issue. The PR description can be updated separately if needed.

<!-- claude-review-cycle -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

"k8s.io/apimachinery/pkg/api/resource"
)

// Mutation defines a mutation that is applied to a pvc Mutator
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling/terminology: "a pvc Mutator" should be "a PVC Mutator" (PVC is an acronym) to match naming used elsewhere in the repo and in the rest of this package's GoDoc.

Suggested change
// Mutation defines a mutation that is applied to a pvc Mutator
// Mutation defines a mutation that is applied to a PVC Mutator

Copilot uses AI. Check for mistakes.
sourcehawk and others added 10 commits March 23, 2026 20:16
Typed editor providing SetStorageRequest, SetAccessModes, SetStorageClassName,
SetVolumeMode, SetVolumeName, and Raw() escape hatch. Follows the existing
editor patterns (DeploymentSpecEditor, ContainerEditor, etc.).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements PersistentVolumeClaim as an Integration primitive with:
- Builder with fluent API for mutations, flavors, status handlers
- Mutator with plan-and-apply pattern (metadata edits, spec edits)
- DefaultFieldApplicator preserving immutable fields (accessModes,
  storageClassName, volumeMode, volumeName) on existing PVCs
- DefaultOperationalStatusHandler (Bound/Pending/Lost → status mapping)
- DefaultSuspendMutationHandler (no-op, PVCs have no runtime state)
- DefaultSuspensionStatusHandler (always Suspended)
- DefaultDeleteOnSuspendHandler (false, preserve data)
- PreserveCurrentLabels and PreserveCurrentAnnotations flavors

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers capabilities, building, default field application (immutable field
preservation), mutations (boolean-gated, version-gated), internal ordering,
editors (PVCSpecEditor, ObjectMetaEditor), convenience methods, flavors,
status handlers, and usage guidance.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates PVC builder with version labels, storage annotations,
conditional large-storage expansion, label/annotation flavors, data
extraction, and suspension. Follows the configmap and deployment
example patterns.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address Copilot review comment: DefaultFieldApplicator had no test
coverage for its immutable field preservation logic. Add tests
validating that:
- New PVCs (empty ResourceVersion) use all fields from desired
- Existing PVCs preserve immutable fields (accessModes,
  storageClassName, volumeMode, volumeName) from current
- Mutable fields (e.g. storage requests) come from desired

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…inFeature

Aligns with the fix applied to the deployment and configmap primitives,
preventing an empty leading feature plan when the mutator helper calls
beginFeature().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add generic.PreserveStatus call in DefaultFieldApplicator to prevent
  status clobbering on update
- Add TestDefaultFieldApplicator_PreservesStatus regression test
- Update pvc.md to accurately describe mutation ordering (no per-feature
  boundaries due to unexported FeatureMutator interface)
- Update DefaultFieldApplicator doc comment to mention Status preservation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align with upstream change in main that exported BeginFeature() on the
FeatureMutator interface.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk sourcehawk force-pushed the feature/pvc-primitive branch from afbb0a0 to dc9f30a Compare March 23, 2026 21:38
@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 25, 2026 16:06
@sourcehawk sourcehawk review requested due to automatic review settings March 25, 2026 16:10
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Comment on lines +55 to +60
func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) {
if edit == nil {
return
}
m.active.metadataEdits = append(m.active.metadataEdits, edit)
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EditObjectMetadata/EditPVCSpecdereferencem.activewithout guaranteeing it’s initialized. If a caller uses a*Mutatordirectly (not via the generic integration flow) and forgets to callBeginFeature(), this will panic at runtime. Consider making the mutator safe-by-default by lazily creating a feature plan when m.active == nil(e.g., callBeginFeature()internally) or by initializing a default active plan inNewMutator` (and then adjusting tests/invariants accordingly).

Copilot uses AI. Check for mistakes.
Comment on lines +69 to +74
func (m *Mutator) EditPVCSpec(edit func(*editors.PVCSpecEditor) error) {
if edit == nil {
return
}
m.active.specEdits = append(m.active.specEdits, edit)
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EditObjectMetadata/EditPVCSpecdereferencem.activewithout guaranteeing it’s initialized. If a caller uses a*Mutatordirectly (not via the generic integration flow) and forgets to callBeginFeature(), this will panic at runtime. Consider making the mutator safe-by-default by lazily creating a feature plan when m.active == nil(e.g., callBeginFeature()internally) or by initializing a default active plan inNewMutator` (and then adjusting tests/invariants accordingly).

Copilot uses AI. Check for mistakes.
@sourcehawk sourcehawk requested a review from Copilot March 25, 2026 16:10
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 2 Complete

Addressed:
None

Intentionally ignored:

  • EditObjectMetadata/EditPVCSpec nil-guard on m.active (lines 60, 74): This is a valid defensive programming concern, but the same pattern exists across all primitives (deployment, configmap) without nil guards. The NewMutator doc comment explicitly states "BeginFeature must be called before registering any mutations," and the framework integration flow guarantees this ordering. Adding a lazy-init or nil guard only to the PVC mutator would be inconsistent with the codebase. If this is desired, it should be addressed as a cross-cutting change across all primitives.

<!-- claude-review-cycle -->

@sourcehawk sourcehawk requested review from Copilot and removed request for Copilot March 25, 2026 16:13
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

- Add panic guards with clear messages when EditObjectMetadata/EditPVCSpec
  are called before BeginFeature, preventing nil-pointer dereferences
- Handle empty VolumeName in Bound status to produce a meaningful reason
- Update docs to use Go constant names (OperationalStatusPending, etc.)
- Add tests for new panic behavior and empty volume name case

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • mutator.go:84 — Added explicit panic messages in EditObjectMetadata and EditPVCSpec when called before BeginFeature(), preventing cryptic nil-pointer dereferences. Added corresponding tests.
  • handlers.go:28 — Handle empty VolumeName when PVC is Bound, falling back to "PVC is bound" instead of "PVC is bound to volume ". Added test case for this scenario.
  • pvc.md:12 — Updated capability table and status table to use Go constant names (OperationalStatusOperational, OperationalStatusPending, OperationalStatusFailing) instead of informal names.

Intentionally ignored:
None

<!-- claude-review-cycle -->

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

sourcehawk and others added 2 commits March 25, 2026 16:46
Reword comments on immutable PVC field setters (accessModes,
storageClassName, volumeMode, volumeName) to clarify that SSA does not
preserve changed values — the API server rejects the patch. Also add
PVCSpecEditor to the Mutation Editors table in primitives.md.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sourcehawk
Copy link
Owner Author

Claude Review Cycle 1 Complete

Addressed:

  • Reworded comments on immutable PVC field setters (SetAccessModes, SetStorageClassName, SetVolumeMode, SetVolumeName) to clarify that SSA does not preserve changed immutable values and that the API server will reject the patch if the value differs.
  • Added PVCSpecEditor to the Mutation Editors table in docs/primitives.md.

Intentionally ignored:
None

<!-- claude-review-cycle -->

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants